test(sc): checkpoint save/restore must not drop prompt groups - #3827
Draft
terrykong wants to merge 12 commits into
Draft
test(sc): checkpoint save/restore must not drop prompt groups#3827terrykong wants to merge 12 commits into
terrykong wants to merge 12 commits into
Conversation
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Ruff dropped the unused re-export from replay_buffer.py; the tests still needed the constant. Import it from its canonical location instead. Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
GRPOSaveState now has trainer_version; SingleControllerActorArgs now has data_plane_checkpoint_metadata. Two unit tests were still asserting the old shapes. Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
The train-pump epilogue now enters the data-plane barrier before clearing consumed samples. Two SC tests built controllers via object.__new__ and had no _data_plane_checkpoint_barrier attribute; the second test hit it once dispatch actually happened, raising AttributeError. Also add data_plane_checkpoint_metadata=None to the SetupTimingMetrics test's actor_args, matching the field _init_ now reads. Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Pausing a run to checkpoint and resuming it should train on the same
prompt groups as never pausing at all. These tests state that as a
property and sweep it over samplers and buffer states.
The matrix lives in two tables so the setups can be read without the
machinery: PASSING in test_checkpoint_no_data_loss.py, EXPECTED_TO_FAIL
in test_checkpoint_no_data_loss_xfail.py. Every xfail is strict, so a
fix turns the row into an XPASS failure and whoever fixed it is told to
move it into the passing table.
Runs on CPU in ~10s. Exercises the real TQReplayBuffer, the real
samplers, the real DataPlaneCheckpointBarrier and the real
NoOpDataPlaneClient save/load; only the tensor converter is stubbed.
Result today: 5 passed, 16 xfailed, 0 xpassed. Two causes:
- in-flight groups are never saved (reserve marks the slot not-ready
and metadata_state_dict skips it), which hits every sampler
- in_order/weight_fifo declare supports_buffer_checkpoint=False, so
no sidecar is written and the restore returns early
Signed-off-by: Terry Kong <terryk@nvidia.com>
Splits the matrix by which bar a case misses, because the two are not
the same thing and should not read the same:
test_checkpoint_regressions.py groups that came back BEFORE this
change and do not now. Real failures,
deliberately not xfail.
..._no_data_loss_xfail.py gaps that were here before too.
xfail(strict=True).
..._no_data_loss.py both bars met.
Verified against the merge base rather than assumed: the old save had no
capability gate (single_controller.py:802), the old restore ran for any
sampler on a matching name (:270), and the old state_dict saved the
committed slots while dropping in-flight ones - its docstring says so.
So every fully generated group used to survive a restart under every
sampler, and in-flight loss is pre-existing.
Result: 29 passed, 4 xfailed, 10 failed. The 10 are in_order and
weight_fifo losing groups that used to come back. Two gated rows pass,
which is the check working - in that scenario the old code recovered
nothing either, so there is nothing to regress.
xfail assertions test presence, not readiness, so they stay valid
whichever way a future partial-group restore is built: a group counts
if the restored buffer knows about it at all, committed or awaiting
completion.
Signed-off-by: Terry Kong <terryk@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
CPU-only unit tests for one property of #3480:
Draft, targeted at
amahishi/sc-tq-native-recoveryrather thanmain, so the tests land with the feature. Tests only — no source changes.Result
Visual walkthrough of all six cases and all eighteen outcomes:
https://terrykong.github.io/gh-pages-poc/terryk/pr-3827-test-matrix.html
The 10 failures are deliberate and are the point of this PR. They are not "tests I could not get working" — they are groups that came back before this change and do not now.
Three files, split by which bar a case misses
There are two bars, and conflating them hides the signal:
test_checkpoint_no_data_loss.pytest_checkpoint_regressions.pytest_checkpoint_no_data_loss_xfail.pyA step backwards should be loud, so the regression file has no xfail marks. A gap that was always there is a missing feature, so those are
xfail(strict=True)— a fix turns them into XPASS failures and the row gets moved intoPASSING.What the previous behaviour actually was
Checked against the merge base rather than assumed:
single_controller.py:802:270— and both sides readself._async_cfg.sampler.name, so it always matches on a same-sampler resumestate_dictsaved the committed slots and dropped in-flight ones, in as many words: "Unready reservations are in-flight rollouts and are dropped, matching legacy semantics."So every fully generated group used to survive a restart under every sampler, and in-flight loss is pre-existing. That is exactly the line between the two files.
The regressions
in_orderandweight_fifodeclaresupports_buffer_checkpoint = False, so no sidecar is written (:981) and the restore returns early (:322):Two gated rows pass, which is the assertion working rather than blanket-failing: in
trained-what-was-ready-leaving-a-holethe only untrained groups are in-flight, so the old code recovered nothing either and there is nothing to regress.test_the_rows_are_still_in_transfer_queue_after_a_gated_restorepasses too — TransferQueue still holds every committed row. The regression is the index, not the tensors, so a fix can be index-only.The pre-existing gap
A group that has not committed is never saved:
reserve()marks the slot not-ready (replay_buffer.py:910) andmetadata_state_dictskips it (:1093). A group is reserved or committed with nothing in between, so "one rollout still running" and "not started" are the same thing to a checkpoint — group 13 inS_PARTIALfinished both rollouts and is dropped anyway.These assert on presence, not on readiness, so they survive whatever partial-group recovery ends up looking like.
recoveredcounts a group if the restored buffer knows about it at all — already committed with its missing rollouts regenerated, or a reserved slot for the run to finish. Either design flips these to XPASS, andstrict=Truemakes that a failure that has to be dealt with.How to read the matrix
Two plain tables, so the setups can be scanned without the machinery:
A scenario reads the way you would say it out loud:
Fidelity
Real
TQReplayBuffer, realreserve/commit, realmetadata_state_dict/load_state_dict, real manifest digest, real samplers viacreate_sampler, realNoOpDataPlaneClient.save_checkpoint/load_checkpoint. The save/restore gate mirrors the two call sites above by reading the sampler's ownsupports_buffer_checkpoint. Only the tensor converter is stubbed, so a scenario can use empty prompt records.Testing
No GPU, no Ray cluster, no TransferQueue process. Collected by the existing
L0_Unit_Tests_Otherlane. Note the repo'saddoptsincludes-x, so a CI run stops at the first regression rather than reporting all 10.Open questions
in_orderwill read it. If it is not deliberate, the tensors are still in TransferQueue and the fix is index-only.S_EVICTEDmodel eviction the way you would? An evicted group is treated as an intentional discard and excluded from what a restore must return.Happy to drop, re-scope, or re-target any of this — the tables are meant to be argued with.